-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update Plaid Link Component #6362
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gonna test this now, but LGTM. There's a small improvement that could be made in a follow up. Just going to give this a test and then merge if everything looks good.
const emitter = new NativeEventEmitter(nativeModule); | ||
this.listener = emitter.addListener('onEvent', this.onEvent.bind(this)); | ||
listener = emitter.addListener('onEvent', (event) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it will work, but I noticed there's also this in the docs
https://github.com/plaid/react-native-plaid-link-sdk#to-receive-onevent-callbacks
Works great on iOS. Testing Android next. |
I think we should CP this to Staging so it goes live sooner. But feel free to remove the label if you think otherwise. |
|
Having some trouble testing Android. I'm not able to create workspace policies for some reason. I think probably there is some issue with my dev environment - working through it now. |
Ok fixed the dev environment issue, but not able to complete the link handoff on Android. I'm just getting this spinner after going through the OAuth flow and it fails with endless spinner when trying to put me back into the app. What should we try next @TomatoToaster ? |
I think this is probably not working because we are not yet passing the Android package |
Hmm which android package do you think we should update? |
Updated steps to test this OAuth flow on iOS only for now. |
Sorry, to clarify, we need to specify an |
FWIW added this and it works on Android for me but we can do this in a follow up. diff --git a/src/libs/API.js b/src/libs/API.js
index d7d8c6d6b..1d5017601 100644
--- a/src/libs/API.js
+++ b/src/libs/API.js
@@ -883,7 +883,7 @@ function Wallet_GetOnfidoSDKToken() {
* @returns {Promise}
*/
function Plaid_GetLinkToken() {
- return Network.post('Plaid_GetLinkToken', {}, CONST.NETWORK.METHOD.POST, true);
+ return Network.post('Plaid_GetLinkToken', {android_package: 'com.expensify.chat'}, CONST.NETWORK.METHOD.POST, true);
} |
@marcaaron I also updated the podfile/ios package information. I had the podfile and workspace file change when I ran I was able to test this on iOS and it worked with the plaid event emitter changes. |
Hmm seems fine to include or not? I don't know much about podfile tbh but unsure why |
I guess one other thing is that I'm a little confused how this is working and wonder if it will actually work on staging. If you read this you'll see it mentions linking to a "blank page" -> https://plaid.com/docs/link/oauth/#ios-sdk-react-native-ios We added this here: App/apple-app-site-association Line 16 in 8d0298c
But are not passing a So why does this still work? I think (but I'm not sure) it's because we don't need the redirect to actually deliver us to that page - just drop us back into the app (which has already handled the success response). Only thing is... I don't see where we registered |
Also interesting as this blurb says we cannot test "app to app" flows in the sandbox -> https://plaid.com/docs/link/oauth/#app-to-app-authentication But the Platypus OAuth option seems to work fine 🤔 |
I'm good to merge this because I don't think it will break anything, but I think to actually test whether this works we need to try a connection that uses OAuth (and I have a good feeling it won't work). When using the "Platypus OAuth" test we don't ever really leave the app on iOS so we don't need a |
Update Plaid Link Component (cherry picked from commit 1a8b4fa)
🚀 Cherry-picked to staging by @marcaaron in version: 1.1.15-21 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
🚀 Deployed to staging by @marcaaron in version: 1.1.16-0 🚀
|
@marcaaron @TomatoToaster Is this QAble by us? I don't think we have access to the Platypus bank account credentials yet. Let us know if this is internal or not 😬 Thanks in advance! |
Ah can we please test that Chase bank credentials are still working for now? Also, there are no credentials for Platypus bank account just enter anything and it should succeed. |
🚀 Deployed to production by @roryabraham in version: 1.1.16-10 🚀
|
1 similar comment
🚀 Deployed to production by @roryabraham in version: 1.1.16-10 🚀
|
Details
Refactors this component as functional so we can use the Plaid Hooks.
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/182956
Tests
Same as QA done locally and android
QA Steps
This should be tested on iOS
Tested On
Screenshots
Screenshots take a lot of the screen so click here to expand/collapse them!
iOS
(scroll to the bottom)